Add Storage and Surrogate Support to the Price-taker class #1633
Conversation
| if self.config.fixed_design_data is not None: | ||
| # The design is fixed, so all the desired quantities are defined as parameters | ||
| for param_name, param_value in self.config.fixed_design_data.items(): | ||
| setattr(self, param_name, Param(initialize=param_value, mutable=True)) |
There was a problem hiding this comment.
Any reason why you chose mutable Params over fixing Vars ?
There was a problem hiding this comment.
Not quite sure which option is preferable. If we define a Var and fix it, it would allow the user to unfix it. If they do that, then the design variable would be unconstrained and it may lead to an unexpected behavior.
I feel that declaring it as a Param would prevent unfixing and it ensures that the design is always fixed. I can change it to fixing Var if that is the convention we are following everywhere.
…to pt-improvements
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1633 +/- ##
==========================================
- Coverage 77.01% 76.82% -0.19%
==========================================
Files 395 395
Lines 63555 63754 +199
Branches 10365 10408 +43
==========================================
+ Hits 48944 48977 +33
- Misses 12171 12335 +164
- Partials 2440 2442 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @declare_process_block_class("StorageModel") | ||
| class StorageModelData(ProcessBlockData): |
There was a problem hiding this comment.
Is there any reason why we don't just have this in a separate file?
| def _add_upper_bound(var_name, bound): | ||
| if isinstance(bound, (Var, Expression)): | ||
| setattr( | ||
| self, | ||
| var_name + "ub_con", | ||
| Constraint( | ||
| expr=getattr(self, var_name) <= bound, | ||
| doc=f"Constrains the maximum value of {var_name}", | ||
| ), | ||
| ) | ||
|
|
||
| else: | ||
| getattr(self, var_name).setub(bound) | ||
|
|
||
| _add_upper_bound("charge_rate", self.config.max_charge_rate) | ||
| _add_upper_bound("discharge_rate", self.config.max_discharge_rate) | ||
| _add_upper_bound("final_holdup", self.config.max_holdup) | ||
| _add_upper_bound("initial_holdup", self.config.max_holdup) |
There was a problem hiding this comment.
If I understand correctly, this _add_upper_bound function essentially provides the upper bound for a variable by adding a constraint (named m.charge_rate_ub_con in the case of charge_rate, for example) when a Var or Expression is provided.
However, when a float, int, etc., is used, this constraint would not be constructed and the upper bound of the Var would just be set directly.
While it seems like a trivial and subtle difference, it also seems less than ideal to allow variation in model structure like this (where one version includes upper bound constraints and another analogous version does not, simply based on input provided).
Is there a reason why we don't just (1) check whether instance is a Var or Expression, and if so, just do setub(pyo.value(bound)) ? The only potential issue that I can foresee with that is if an indexed Var or Expression is passed in, but haven't checked. Are there other reasons?
|
@radhakrishnatg, @adam-a-a: Moving this to the Nov release. |
Summary/Motivation:
This PR adds new features to the price-taker class. This is WIP. This PR needs to be merged after merging #1579
Changes proposed in this PR:
pd.DataFrameanddict, respectively.StorageModelis found in the flowsheet instance, then linking constraints are now added automatically.Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: